caddyhttp: reject HTTP listeners that use admin API port#7868
Conversation
Expose LocalAdminPort() so the HTTP app can detect listener addresses that conflict with the local admin endpoint during validation. When admin is disabled, the check is skipped so HTTP servers may use the default admin port. When admin listens on a custom port, only that port is protected. Fixes: caddyserver#7053 Signed-off-by: Ebrahim Nejati <ebrahim@nejati.net>
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
steadytao
left a comment
steadytao
left a comment
There was a problem hiding this comment.
Comparing only the numeric port is too broad because LocalAdminPort discards the admin listener's network and host. For example, an admin listener on 127.0.0.1:2019 does not necessarily conflict with an HTTP listener bound to a different concrete address on port 2019, and this also cannot represent Unix or abstract-socket admin listeners correctly. Could we preserve the complete resolved NetworkAddress and check actual listener overlap instead?
@steadytao I thought about this at first too. I decided to keep the simple check because checking the exact address makes the code complex and might confuse the user. I will try to find a clean way to check the exact overlap using the NetworkAddress. |
Adjusted related logic in the app validation to check for overlapping listener addresses with the admin API. Added a new test to ensure non-overlapping admin addresses are allowed. Fixes: caddyserver#7053 Signed-off-by: Ebrahim Nejati <ebrahim@nejati.net>
|
@steadytao Tell me if it's good so I can squash it afterwards |
|
Just a quick note that mentioning me multiple times will not make me any less busy then I am 😅 Additionally, it is the weekend currently. I will take a look but please keep in mind to be respectful in the future, I do believe my status should warn upon mentioning me that I will be slow to respond. |
steadytao
left a comment
steadytao
left a comment
There was a problem hiding this comment.
This is closer but the overlap helper still has incorrect wildcard, loopback, Unix-socket and network-family cases.
Refactored the OverlapsWith method in NetworkAddress. Added new test cases to validate overlapping and non-overlapping scenarios. Signed-off-by: Ebrahim Nejati <ebrahim@nejati.net>
Improved the hostsOverlap method to handle unspecified addresses more accurately. Signed-off-by: Ebrahim Nejati <ebrahim@nejati.net>
|
This ended up scope-exploding once I looked into the listener and reload lifecycle more closely so I took over the remaining changes to make sure the validation, reload behaviour and tests are correct. Sorry for the sizeable follow-up on what initially looked like a small fix. |
There was a problem hiding this comment.
Could I get one of you to double check this @mholt or @francislavoie?
3 participants
Expose LocalAdminPort() so the HTTP app can detect listener addresses that conflict with the local admin endpoint during validation.
When admin is disabled, the check is skipped so HTTP servers may use the default admin port. When admin listens on a custom port, only that port is protected.
Fixes: #7053
Assistance Disclosure
The code was co-authored with Cursor. I manually tested the change and revised the implementation afterwards.